chore: update pubsub interface, multiaddr and remove protons #89

vasco-santos · 2021-04-07T15:43:35Z

This PR adds some minor tweaks following up from feedback on #87 and updates multiaddr

Moreover, updating all the dependencies and removing protons in favour of protobufjs for smaller bundles and avoiding bundle problems with web-encoding

Most of the changes are auto generated files

Needs:

Unblocks:

vasco-santos · 2021-04-07T17:57:25Z

src/pubsub/index.js

   * @returns {Uint8Array} message id as bytes
   */
  getMsgId (msg) {
    const signaturePolicy = this.globalSignaturePolicy
    switch (signaturePolicy) {
      case SignaturePolicy.StrictSign:
+        // @ts-ignore seqno is optional in protobuf definition but it will exist


Protobuf definition in libp2p specs has this as optional, however it is basically an outdated thing...

vasco-santos · 2021-04-07T17:58:50Z

src/pubsub/index.js

      receivedFrom: from,
      data: message,
      topicIDs: [topic]
    }

    // ensure that the message follows the signature policy
    const outMsg = await this._buildMessage(msgObject)
-    msgObject = utils.normalizeInRpcMessage(outMsg)
+    // @ts-ignore different type as from is converted


Because of the signatures, we have this "hack" where we also add the receivedFrom property and from is converted to a different type from the protobuf. Of couse ts does not like this, but I did not want to change this inner logic in this PR

vasco-santos · 2021-04-08T15:54:55Z

src/pubsub/tests/two-nodes.js

@@ -62,7 +62,9 @@ module.exports = (common) => {
          expect(psB.peers.size).to.equal(1)
          expectSet(psB.topics.get(topic), [psA.peerId.toB58String()])
          expect(changedPeerId.toB58String()).to.equal(first(psB.peers).id.toB58String())
-          expect(changedSubs).to.be.eql([{ topicID: topic, subscribe: true }])


With the type object we cannot check this deep eql anymore

rvagg · 2021-04-10T09:51:16Z

package.json

@@ -47,7 +54,7 @@
  },
  "homepage": "https://github.com/libp2p/js-interfaces#readme",
  "dependencies": {
-    "@types/bl": "4.1.0",
+    "@types/bl": "^4.1.0",


probably should be "^5.0.0" now, Alex updated it and it was released yesterday, bl@5 has been out for a week or so too thanks to Alex as well

bl seems like a weird thing to use at the interface level, perhaps it's something we can pull out in future?

It's only used in one place and from the type def it looks like it's only necessary because the exported BufferList can't be treated as a Uint8Array based on the type heirachy.

rvagg · 2021-04-10T09:52:34Z

src/pubsub/index.js

@@ -396,7 +396,7 @@ class PubsubBaseProtocol extends EventEmitter {
    if (msgs.length) {
      // @ts-ignore RPC message is modified
      msgs.forEach((message) => {
-        if (!(this.canRelayMessage || message.topicIDs.some((/** @type {string} */ topic) => this.subscriptions.has(topic)))) {
+        if (!(this.canRelayMessage || (message.topicIDs && message.topicIDs.some((topic) => this.subscriptions.has(topic))))) {


oo, was this a bug that types picked up?

This is because the protobuf definition does not state topicIDs as required. As far as I know, all implementations send an empty arrau and not undefined and this has not been a problem. But could be as it is not spec'ed as required

rvagg · 2021-04-10T09:55:27Z

src/pubsub/message/rpc.d.ts

+    }
+
+    /** Properties of a Message. */
+    interface IMessage {


why are the properties here all both optional and allow null?

This is generated from the protobuf spec for pubsub where no part of the message is required. But basically, "Optional fields may be undefined depending on the signature policy and message ID function."

rvagg

sgtm (didn't review the two generated files, I'm assuming there's not much to say there)

The only major thing that stands out is the liberal use of ? combined with |null in the types. I suppose if this is just an initial go at external facing interfaces then that might an ok start, but it seems to suggest some looseness that maybe could be tightened up later?

vasco-santos · 2021-04-11T17:05:39Z

The only major thing that stands out is the liberal use of ? combined with |null in the types. I suppose if this is just an initial go at external facing interfaces then that might an ok start, but it seems to suggest some looseness that maybe could be tightened up later?

As we reuse the same definition for several types of pubsub messages supported, the protobuf states them as optional, which makes them possibly null. More details in review comments

achingbrain · 2021-04-12T07:50:42Z

src/pubsub/message/topic-descriptor.proto

 // topicCID = cid(merkledag_protobuf(topicDescriptor)); (not the topic.name)
 message TopicDescriptor {
  optional string name = 1;
  optional AuthOpts auth = 2;
-  optional EncOpts enc = 2;
+  optional EncOpts enc = 3;


yes, this was awkward! Go and the spec were right, but I guess there is no one using topic descriptors in JS so far
Spec: https://github.com/libp2p/specs/tree/master/pubsub#the-topic-descriptor
Go: https://github.com/libp2p/go-libp2p-pubsub/blob/master/pb/rpc.proto#L61

Types catching all the bugs 🎉

achingbrain · 2021-04-12T07:51:20Z

src/pubsub/peer-streams.js

@@ -9,6 +9,7 @@ const log = Object.assign(debug('libp2p-pubsub:peer-streams'), {
 /** @type Events */
 const EventEmitter = require('events')

+// @ts-ignore TODO: https://github.com/alanshaw/it-length-prefixed/pull/15


This has been merged, can be removed?

I am waiting on releasing this: alanshaw/it-length-prefixed#16
Otherwise tests fail

package.json

Co-authored-by: Alex Potsides <alex@achingbrain.net>

vasco-santos force-pushed the chore/update-pubsub-interface branch 2 times, most recently from 5204d56 to a372212 Compare April 7, 2021 15:52

chore: update pubsub interface

0cb7096

vasco-santos force-pushed the chore/update-pubsub-interface branch from a372212 to 0cb7096 Compare April 7, 2021 16:09

vasco-santos marked this pull request as ready for review April 7, 2021 17:38

vasco-santos mentioned this pull request Apr 7, 2021

chore: update deps and remove protons ChainSafe/js-libp2p-gossipsub#153

Merged

1 task

chore: update pubsub tests

0b4fd03

vasco-santos mentioned this pull request Apr 7, 2021

chore: update deps libp2p/js-libp2p-floodsub#114

Merged

1 task

vasco-santos changed the title ~~chore: update pubsub interface~~ chore: update pubsub interface and remove protons Apr 7, 2021

vasco-santos requested a review from achingbrain April 7, 2021 19:38

chore: update multiaddr to 9

d224d15

vasco-santos commented Apr 8, 2021

View reviewed changes

This was referenced Apr 8, 2021

chore: update multiaddr to 9 libp2p/js-libp2p-utils#17

Merged

chore: config typescript libp2p/js-libp2p#904

Merged

vasco-santos requested a review from hugomrdias April 8, 2021 16:51

vasco-santos changed the title ~~chore: update pubsub interface and remove protons~~ chore: update pubsub interface, multiaddr and remove protons Apr 8, 2021

rvagg reviewed Apr 10, 2021

View reviewed changes

rvagg approved these changes Apr 10, 2021

View reviewed changes

rvagg mentioned this pull request Apr 10, 2021

[CoreTypes] Tracking protocol/w3dt-sudo#17

Open

77 tasks

achingbrain reviewed Apr 12, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

vasco-santos and others added 2 commits April 12, 2021 10:12

chore: update it-handshake

eb16409

Co-authored-by: Alex Potsides <alex@achingbrain.net>

chore: use npm on ci

ee340d8

vasco-santos force-pushed the chore/update-pubsub-interface branch from 6997a8a to ee340d8 Compare April 12, 2021 08:28

chore: update it-length-prefixed

308a3ec

vasco-santos merged commit 8a6d3d0 into master Apr 12, 2021

vasco-santos deleted the chore/update-pubsub-interface branch April 12, 2021 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update pubsub interface, multiaddr and remove protons #89

chore: update pubsub interface, multiaddr and remove protons #89

vasco-santos commented Apr 7, 2021 •

edited

Loading

vasco-santos Apr 7, 2021

vasco-santos Apr 7, 2021

vasco-santos Apr 8, 2021

rvagg Apr 10, 2021

achingbrain Apr 12, 2021

rvagg Apr 10, 2021

vasco-santos Apr 11, 2021

rvagg Apr 10, 2021

vasco-santos Apr 11, 2021 •

edited

Loading

rvagg left a comment

vasco-santos commented Apr 11, 2021

achingbrain Apr 12, 2021

vasco-santos Apr 12, 2021

vasco-santos Apr 12, 2021

achingbrain Apr 12, 2021

vasco-santos Apr 12, 2021

chore: update pubsub interface, multiaddr and remove protons #89

chore: update pubsub interface, multiaddr and remove protons #89

Conversation

vasco-santos commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

rvagg left a comment

Choose a reason for hiding this comment

vasco-santos commented Apr 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Apr 7, 2021 •

edited

Loading

vasco-santos Apr 11, 2021 •

edited

Loading